Skip to content

fix: address coderabbit comments from #31 #32

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
calthejuggler merged 1 commit into from
Jan 15, 2026
Merged

fix: address coderabbit comments from #31 #32

calthejuggler merged 1 commit into from
Jan 15, 2026

Conversation

@calthejuggler
Copy link
Contributor

@calthejuggler calthejuggler commented Jan 14, 2026 •
edited by coderabbitai bot
Loading

This PR addresses some review comments from PR #31. As that PR is a simple reversion - I have put these here for ease of reviewing.

Summary by CodeRabbit

  • Bug Fixes

    • Enforced parameter validation during context creation.
    • Improved resilience when checking client operation status to avoid errors if internal state is absent.

  • Breaking Changes

    • Client initialisation now requires a configuration parameter.
    • Removed deprecated public configuration/context-access methods and cleaned up unused internal configuration variables.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Jan 14, 2026 •
edited
Loading

Caution

Review failed

The pull request is closed.

Walkthrough

The PR removes unused public API and class variables: ABSmartly.configure_client and its associated initialization use, ABSmartlyConfig#context_data_provide, and @@init_config from the Absmartly module. ABSmartly#initialize no longer resets @@init_http. ABSmartly#create_context_with(config, data) now calls validate_params(config) before creating a Context. Client#initialize was changed to require config, and Client#success? now safely handles a nil @promise.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

Poem

🐇 I hopped through code at break of day,
Tucked unused bits and tucked them away.
Configs now firm, params checked with care,
Fewer surprises, lighter the air.
A rabbit's tidy patch — hop, skip, hooray! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: address coderabbit comments from #31' is directly related to the PR's objective of addressing review comments from PR #31, and accurately summarises the primary purpose of the changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5739887 and 5ae5238.

📒 Files selected for processing (4)
  • lib/a_b_smartly.rb
  • lib/a_b_smartly_config.rb
  • lib/absmartly.rb
  • lib/client.rb

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Comment @coderabbitai help to get the list of available commands and usage tips.

@calthejuggler
Copy link
Contributor Author

calthejuggler commented Jan 14, 2026

@coderabbitai full review

@coderabbitai
Copy link

coderabbitai bot commented Jan 14, 2026

✅ Actions performed

Full review triggered.

cwmpinho
cwmpinho approved these changes Jan 15, 2026
View reviewed changes
Copy link

@cwmpinho cwmpinho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Tested locally that exposure event is registered correctly

Base automatically changed from bugfix/revert-broken-mutex-deadlock-changes to main January 15, 2026 12:43
@calthejuggler calthejuggler force-pushed the bugfix/coderabbit-comments branch from 5739887 to 5ae5238 Compare January 15, 2026 12:43
@calthejuggler calthejuggler merged commit 74e8e4d into main Jan 15, 2026
6 of 7 checks passed
@calthejuggler calthejuggler deleted the bugfix/coderabbit-comments branch January 15, 2026 12:45
@coderabbitai coderabbitai bot mentioned this pull request Jan 16, 2026
Merged
fcbr added a commit to catawiki/absmartly-ruby-sdk that referenced this pull request Jan 22, 2026
@fcbr @hermeswaldemarin
@marcio-absmartly @yektt @calthejuggler
Update fork (#4)
8b0fa52 
* Remove unused lib/absmartly/variant_assigner

* Support create context with given context data.

* Config refactor

* Add ruby version matrix to the CI

* remove bigdecimal as a dependency

* Fix relative path in DefaultContextEventHandler.

* Fix class inheritance in DefaultContextEventHandler.

* Update version

* fix: merge context attributes when setting them (absmartly#29)

* revert: undo config refactor from PR absmartly#27 (fixes deadlock) (absmartly#31)

* fix: address coderabbit comments (absmartly#32)

* fix: allow http client configuration from configure_client (absmartly#33)

* feat: add global cutom event logger  (absmartly#34)

---------

Co-authored-by: hermeswaldemarin <hermes@absmartly.com>
Co-authored-by: Márcio Martins <77632139+marcio-absmartly@users.noreply.github.com>
Co-authored-by: Hermes Waldemarin Neto <hermes.waldemarin@gmail.com>
Co-authored-by: Dhruv Paranjape <d.paranjape@catawiki.nl>
Co-authored-by: yektt <r.yekt.93@gmail.com>
Co-authored-by: Cal Courtney <calthejuggler@gmail.com>
Co-authored-by: Cal Courtney <cal@absmartly.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marcio-absmartly marcio-absmartly marcio-absmartly approved these changes

+1 more reviewer

@cwmpinho cwmpinho cwmpinho approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@calthejuggler @marcio-absmartly @cwmpinho